Skip to content

[v7r2] Move to a src/ layout#4858

Merged
atsareg merged 4 commits into
DIRACGrid:integrationfrom
chrisburr:src-layout
Dec 9, 2020
Merged

[v7r2] Move to a src/ layout#4858
atsareg merged 4 commits into
DIRACGrid:integrationfrom
chrisburr:src-layout

Conversation

@chrisburr

@chrisburr chrisburr commented Dec 7, 2020

Copy link
Copy Markdown
Member

The first step in implementing what was discussed at the last BiLD. Obviously this PR touches a lot of files but it should be quite easy to review as there are very few actual changes. It is split into three commits:

  • 79d6af7: This is the big diff which is simply moving the files:
git mv __init__.py AccountingSystem ConfigurationSystem Core DataManagementSystem FrameworkSystem Interfaces MonitoringSystem ProductionSystem RequestManagementSystem Resources ResourceStatusSystem StorageManagementSystem TransformationSystem Workflow WorkloadManagementSystem src/DIRAC/
  • 7fa80a7: Fixes required to get the CI passing again
  • 5fb66e6: Fixes for some pylint errors which show up in the certification test files

First part of #4774

BEGINRELEASENOTES

*Python 3
NEW: Moved to a src/ repository layout

ENDRELEASENOTES

# gracefully, make them non-daemonic and use a suitable
# signalling mechanism such as an Event."
- pytest --no-cov -k 'not test_BaseType_Unicode and not test_nestedStructure and not testLockedClass'
# - pytest --no-cov src/DIRAC/Core/Security/test

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chaen In the previous Python 3 merge request I had missed this test. I tried adding it here but it fails in a non obvious way. Would you be able to take a look at some point?

(if you can you should work on top of this branch as I fixed all except one of the failures already)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you mean the Security tests ? Yes, these are nasty, because they unload some modules from memory in order to do a pyGSI/M2Crypto comparison. I think they can be simplified, and then not run anymore as a specific tests. I'll have a look !

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that seems to be an M2Crypto issue. I have opened a bug report. I suspect that the problem lies in the C-API, probably some mess with str/bytes but since I have no knowledge of that, I will wait to see if there is a reaction before embarking in fixing it myself

https://gitlab.com/m2crypto/m2crypto/-/issues/287

@fstagni

fstagni commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

I have seen you moved tests/Utilities → src/DIRAC/tests/Utilities/
I thought the tests directory would have not been moved.

@chrisburr

Copy link
Copy Markdown
Member Author

I have seen you moved tests/Utilities → src/DIRAC/tests/Utilities/
I thought the tests directory would have not been moved.

Unfortunately it's a bit messy as some bits of test code are being imported as:

from DIRAC.tests.Utilities import something

There are other ways of fixing this this and I'm happy to do it differently if you have a strong opinion? I think the correct solution is probably to tidy up the tests a bit but that's a task for another PR.

@fstagni

fstagni commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

Alright. #4859

@andresailer

Copy link
Copy Markdown
Contributor

There is a lot of documentation out there that points to the location of dirac-install.py where it is at the moment in the integration branch on github, isn't there?

@fstagni

fstagni commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

Excellent observation @andresailer

I re-launch: what if we move dirac-install.py in management repo? It has always been a script not depending from any specific release, and, in a way, this PR is also a first step towards retirement of dirac-install script.

@chrisburr

Copy link
Copy Markdown
Member Author

(In the current model of packaging and updating) Is there a use case for dirac-install.py being included in the installation? If not, I think it's a great idea to move it over to DIRACGrid/management so we can have only a single breakage from the URL changing.

@atsareg

atsareg commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

dirac-install is used to upgrade the already existing installation. In this case typically it is taken from the installation itself. However, it is the same dirac-install (may be a different version) as the one for an initial installation. So, moving it to management does not harm

@chrisburr

Copy link
Copy Markdown
Member Author

dirac-install is used to upgrade the already existing installation

Won't moving it to management break this workflow as it will no longer exist in DIRAC/Core/scripts? Or is the suggestion to copy it and use the file in management only during manual installations?

@fstagni

fstagni commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

Well, we can replace the dirac-install script that's here with a script that just downloads the dirac-install that is in management and run it. This would BTW make sure that we don't run into backward incompatibilities.
There are other solutions of course.

@atsareg

atsareg commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

Is the dirac-distribution.py and the related stuff updated in the management project to take this change into account ?

@atsareg

atsareg commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

This change is probably not so complex but still can have lots of repercussions here and there. So, I would suggest to make it the first one in v7r3 release. I mean I will make rel-v7r2 branch with the current integration contents which should be just certified for the release. Then I merge this one into integration straight away. What do you think ?

@fstagni

fstagni commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

I think that this PR can be merged as it is in v7r2. For the move of dirac-install out of this repo we can wait v7r3.

@andresailer

Copy link
Copy Markdown
Contributor

Can we please keep a copy of dirac-install at its current location, and not move it a week before the end of year break? Maybe add a deprecation print out at the end of the script? Ideally already putting one in diracgrid/management and referring to that location in the previously mentioned deprecation message?

@fstagni

fstagni commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

I opened issue #4860 to discuss the "dirac-install move" topic, as that won't be part of this PR. Here, what should be addressed is only this:

There is a lot of documentation out there that points to the location of dirac-install.py where it is at the moment in the integration branch on github, isn't there?

@chrisburr

chrisburr commented Dec 9, 2020

Copy link
Copy Markdown
Member Author

This change is probably not so complex but still can have lots of repercussions here and there. So, I would suggest to make it the first one in v7r3 release.

I would really like to include this and the entry_points PR. Without them it will be impossible for me to keep maintaining the conda-forge package without considerable work. I also think it will delay DIRAC moving to Python 3 by at least 6 months as we can't start thinking about properly testing extensions without these PRs. I admit it's likely to cause some disruption for the certification hackathons but delaying v7r2 release by a couple of weeks seems preferable to me, especially as no one seems to desperate to use the other features in v7r2?

For the move of dirac-install out of this repo we can wait v7r3.

I agree, I'll add a copy of it now.

@atsareg

atsareg commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

I guess I will not be able to compile the v7r2 distribution tar balls if this PR is merged ?

@chrisburr

Copy link
Copy Markdown
Member Author

I guess I will not be able to compile the v7r2 distribution tar balls if this PR is merged ?

@atsareg Can you point me to the docs on exactly how you do this so I can check?

@atsareg

atsareg commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

@fstagni

fstagni commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

Basically: you would need to modify a line or 2, probably here: https://github.com/DIRACGrid/management/blob/master/dirac-distribution/dirac-create-distribution-tarball.py#L463 (I only had a very quick look) and then the docker container will need to be re-created. To force that, temporary change this line: https://github.com/DIRACGrid/management/blob/master/.github/workflows/images-creator.yml#L14

@chrisburr

Copy link
Copy Markdown
Member Author

I've just opened the PR: DIRACGrid/management#28

I've tested it locally with a hack to make it clone this branch instead of the actual tag and it seems to work.

@fstagni

fstagni commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

Yes, merged. The containers are created here: https://github.com/DIRACGrid/management/actions/runs/410422572

@atsareg atsareg merged commit ebf6678 into DIRACGrid:integration Dec 9, 2020
@chrisburr chrisburr deleted the src-layout branch December 9, 2020 11:09
@chrisburr

Copy link
Copy Markdown
Member Author

I've checked and the new image works as expected.

@atsareg Can you ping me once you've uploaded the tarballs so I can run some checks before tomorrow's hackathon rather than risking it needing to be cancelled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants